Repository navigation
feat(vault): Added 1Password secrets provider - #5154
bjerringgaard wants to merge 6 commits into
Conversation
|
@narcisonunez Tests have now been added. |
|
I tested this branch against a real 1Password account on a local instance, since it looked like nobody had yet. The implementation itself works well, but there is a blocking issue that only shows up at runtime. Blocking: the enum migration is gone, so creating a provider failsThe branch adds The original commit ALTER TYPE "public"."VaultProviderType" ADD VALUE 'onepassword';Commit Reproduced on a fresh instance built from this branch: Creating a 1Password provider then fails with I confirmed the causality: applying just This is invisible in review — the TypeScript is perfectly consistent — and only surfaces the first time someone actually creates a 1Password provider. The fix is to rebase on Everything else works against a real accountWith the enum value present, tested end to end against a real 1Password Environment and service account:
Both Two minor observations, not blockers
One note on scopeBoth open requests for this feature ask for the other 1Password model. #1192 asks specifically for That's not a criticism of this PR, which does what it says. I'd be glad to send a follow-up adding |
3040236 to
b0c7790
Compare
|
@mitc-gjuge On Minor observations vaultFetchWithTimeout On the scope note |
|
Thanks for picking these up so quickly — I re-tested the migration fix this morning and it lands correctly. On a database rebuilt from your branch the enum comes out as testConnection — you're right, and I'd drop it. Testing that the credentials can reach the environment is the correct contract; whether that environment currently holds variables is the user's business, and an empty one is a legitimate state. My own confusion came from having an empty environment on the first run, which is an argument for a hint at most, not for changing what the call verifies. vaultFetchWithTimeout — I looked for a better answer and I don't think there is one. In So On scope — your reasoning holds, and I'd not want it bolted onto this PR. Environments are what 1Password is steering people towards, the per-environment access boundary is a real advantage over a broad service account, and shipping the narrower thing first is the right shape for review. The one thing I'd flag is that the two open requests ask for the other model: #1192 spells out None of that argues against merging this. It argues for a follow-up, which I'm happy to write once this lands: same |
|
Heads-up: merging #5257 put this PR in conflict — Worth flagging one subtlety before you rebase. Renaming the file to if (!lastDbMigration || Number(lastDbMigration.created_at) < migration.folderMillis)and yours is now earlier than canary's:
So on any database that has already run canary's Regenerating after the rebase — |
30a2eec to
345bb5b
Compare
345bb5b to
292e061
Compare
|
@narcisonunez — small heads-up on #5154, if you have a moment. Both workflows on Your 24 August review asked for tests in |
292e061 to
226c18c
Compare
|
Amazing PR. Can't wait! Will this coming version also enable OP password references so the environments are not necessary? |
|
Not in this one — it reads 1Password Environments only, so an Environment ID is
|
|
@appscaptain The first version of this PR, is about adding the env feature 1password themselves are pushing for. I plan on creating a follow up PR, adding the "op://" handler more or less right after this PR gets merged. |
|
@narcisonunez Would it be possible to have the workflows run on this PR once again? |
|
Sounds good with the plan. Looking very much forward to it. The reason why I am asking for the op support is that: So the process of creation and maintenance of environment data becomes a mess I think So while I like 1Password idea of the environments, due to its implementation with all these limitations, it is just way more practical to simply use OP:// references as this overcomes every single one of those issues. |
226c18c to
7dcceb2
Compare
|
@narcisonunez @Siumauricio any updates? |
|
We'd use this in production too — all our secrets already live in 1Password, so this PR (and the |
|
Yeah, also really need this. As I couldn't wait I merged this into my own Dokploy install across 3 servers a few weeks ago and it has been working perfectly with the 1Password environments - no issues whatsoever. But really missing the op:// support though, as that would be way easier to maintain than Environments. |
What is this PR about?
Added 1Password as an option for the vault-providers.
Making use of 1password's newer "Environments" feature.
Relying on 1Password's Service Account Authentication
Checklist
Before submitting this PR, please make sure that:
canarybranch.Issues related (if applicable)
closes #1777
closes #1192
Greptile Summary
The PR adds 1Password Environments as a vault provider using service-account authentication.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously unbounded 1Password SDK operations now reject through the shared vault timeout wrapper.
Reviews (4): Last reviewed commit: "fix: added tests" | Re-trigger Greptile
Context used: